reshape and document the dht-cache #36

lu-zero · 2023-07-10T20:09:40Z

Reduce the public API
Document it

codecov · 2023-07-20T11:46:42Z

Codecov Report

Merging #36 (c4000eb) into master (284e00e) will increase coverage by 8.30%.
Report is 7 commits behind head on master.
The diff coverage is 82.17%.

❗ Current head c4000eb differs from pull request most recent head ff3cc90. Consider uploading reports for the commit ff3cc90 to get more accurate results

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   71.72%   80.02%   +8.30%     
==========================================
  Files          12       17       +5     
  Lines        1963     2959     +996     
==========================================
+ Hits         1408     2368     +960     
- Misses        555      591      +36

Files	Coverage Δ
dht-cache/src/domopersistentstorage.rs	`100.00% <100.00%> (+26.72%)`	⬆️
dht-cache/src/utils.rs	`100.00% <ø> (ø)`
dht-cache/src/data.rs	`94.11% <94.11%> (ø)`
dht-cache/src/lib.rs	`0.00% <0.00%> (ø)`
src/domobroker.rs	`93.53% <60.00%> (-0.25%)`	⬇️
dht-config/src/lib.rs	`8.62% <0.00%> (+8.62%)`	⬆️
src/main.rs	`0.00% <0.00%> (-1.08%)`	⬇️
dht-cache/src/domolibp2p.rs	`93.33% <90.00%> (+6.54%)`	⬆️
dht-cache/src/cache/local.rs	`94.97% <94.97%> (ø)`
dht-cache/src/domocache.rs	`68.85% <41.17%> (+23.86%)`	⬆️
... and 2 more

... and 2 files with indirect coverage changes

dht-cache/src/cache.rs

dht-cache/src/dht.rs

github-actions · 2023-08-11T07:10:34Z

dht-cache/src/cache/local.rs

+        let mut cache = self.0.write().await;
+
+        if let Some(storage) = cache.store.as_mut() {
+            storage.store(&elem).await;


🚫 [clippy] _{reported by reviewdog 🐶}

error: this expression creates a reference which is immediately dereferenced by the compiler --> dht-cache/src/cache/local.rs:87:27 | 87 | storage.store(&elem).await; | ^^^^^ help: change this to: `elem` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow = note: `-D clippy::needless-borrow` implied by `-D warnings`

dht-cache/src/cache/local.rs

dht-cache/src/cache.rs

dodomorandi

I still need to finish reviewing, I have two major concerns about use of unsafe and select! other things are minor.

dodomorandi · 2023-08-21T07:48:02Z

dht-cache/src/cache/local.rs

+
+            for (topic_uuid, value) in map_topic_name.iter() {
+                topic_uuid.hash(state);
+                value.to_string().hash(state);


This is pretty bad, because it requires N^2 allocations. Considering that a hash fn should be fast, we have a potential problem, even from a cryptographic standpoint.

We could call hash over the larger map, I moved the code around but I did not check if the hash value would change.

Is there a way to easily split these comments into a separated issue? Obviously, for me it is easy to just check the diff, but I agree that the aim here is a bit different. If we can easily split issues that should be fixed in different PRs, it would be great.

Sadly there isn't, it would be a nice feature though.

I'd do a round of performance improvements after we are good with the API rework :)

dodomorandi · 2023-08-21T07:58:03Z

dht-cache/src/cache/local.rs

+        let topic_name = elem.topic_name.clone();
+        let topic_uuid = &elem.topic_uuid;
+
+        let topic = cache.mem.entry(topic_name).or_default();


Maybe we could consider using the contains_key + eventual allocation and insertion, instead of using the entry API. Last time I checked this approach for a HashMap (and it was convenient to approach the problem in this way), but I never did any benchmark

dht-cache/src/cache/local.rs

dodomorandi · 2023-08-21T08:19:40Z

dht-cache/src/cache/local.rs

+    pub fn new(topic: &str, cache: LocalCache) -> Self {
+        Self {
+            topic: topic.to_owned(),


Suggested change

pub fn new(topic: &str, cache: LocalCache) -> Self {

Self {

topic: topic.to_owned(),

pub fn new(topic: impl Into<String>, cache: LocalCache) -> Self {

Self {

topic: topic.into(),

dodomorandi · 2023-08-21T08:20:37Z

dht-cache/src/cache/local.rs

+    pub fn with_uuid(mut self, uuid: &str) -> Self {
+        self.uuid = Some(uuid.to_owned());


Suggested change

pub fn with_uuid(mut self, uuid: &str) -> Self {

self.uuid = Some(uuid.to_owned());

pub fn with_uuid(mut self, uuid: impl Into<String>) -> Self {

self.uuid = Some(uuid.into());

dht-cache/src/cache.rs

Should make easier testing the network component using libp2p_swarm_test.

The resend task would not interfere with the rest this way.

Co-authored-by: Edoardo Morandi <morandidodo@gmail.com>

dodomorandi

Ok, just a major concern, most of the things are minor or considerations.

dht-cache/src/cache/local.rs

dht-cache/src/dht.rs

dodomorandi · 2023-08-22T07:47:20Z

dht-cache/src/dht.rs

+                        return swarm
+                    }
+                }
+                ev = swarm.select_next_some() => {


This seems to be cancel-safe, therefore the select! should be fine. However, this characteristics does not seem to be documented anywhere, therefore I don't think we can assume that future non-breaking versions of libp2p will have this specific behavior. On the other hand, given that the implementation of Stream currently forwards to the internal sync poll_next_event function, it is very plausible that swarm.select_next_some() will continue to be cancel-safe.

If you want to be sure, the resulting future could be saved and pinned in order to keep it across different iterations, but it is noisy and probably it is not worth at this point. What do you think?

dht-cache/src/dht.rs

dht-cache/src/domocache.rs

Otherwise it would infiniloop it send the volatile message when only one of the two peers is ready.

dodomorandi · 2023-08-25T07:21:08Z

dht-cache/src/dht.rs

+                    let peers: Vec<_> = check_peers(&mut swarm);
+                        if !peers.is_empty() &&
+                        ev_send.send(Event::Ready(peers)).is_err() {
+                            return swarm;
+                    }


Formatting:

Suggested change

let peers: Vec<_> = check_peers(&mut swarm);

if !peers.is_empty() &&

ev_send.send(Event::Ready(peers)).is_err() {

return swarm;

}

let peers: Vec<_> = check_peers(&mut swarm);

if !peers.is_empty() && ev_send.send(Event::Ready(peers)).is_err() {

return swarm;

}

lu-zero force-pushed the dht-cache-reshape branch from 7ecbc1b to 766a1cd Compare August 9, 2023 11:26

github-actions bot reviewed Aug 9, 2023

View reviewed changes

dht-cache/src/cache.rs Outdated Show resolved Hide resolved

dht-cache/src/cache.rs Outdated Show resolved Hide resolved

dht-cache/src/dht.rs Outdated Show resolved Hide resolved

github-actions bot reviewed Aug 11, 2023

View reviewed changes

lu-zero force-pushed the dht-cache-reshape branch 2 times, most recently from 185cc26 to 6a3b9fc Compare August 11, 2023 14:11

github-actions bot reviewed Aug 14, 2023

View reviewed changes

dht-cache/src/cache.rs Outdated Show resolved Hide resolved

lu-zero force-pushed the dht-cache-reshape branch 5 times, most recently from 8898e9e to 40306f0 Compare August 18, 2023 11:27

github-actions bot reviewed Aug 18, 2023

View reviewed changes

dht-cache/src/cache.rs Show resolved Hide resolved

lu-zero force-pushed the dht-cache-reshape branch 2 times, most recently from 0132c71 to 1837b6c Compare August 18, 2023 13:28

Luni-4 requested a review from ddeguglielmo August 18, 2023 15:50

dodomorandi requested changes Aug 21, 2023

View reviewed changes

lu-zero force-pushed the dht-cache-reshape branch from 0a87eb6 to 0bb9a1c Compare August 21, 2023 10:49

lu-zero added 12 commits August 21, 2023 18:46

Document dht-cache

53331ba

Hide unneeded API surface

f175b75

Re-export to the root of dht-cache

66c1af6

Initial error type

f9c876b

Simplify the DomoEvent handling

c4f778e

wip: Add get_peers_stats

308d5b6

Hide even more of the DomoCache

390743a

Move the data structures in a separate module

505fd2e

Derive Default for DomoCacheElement

a9a8aa5

Move the rsa key generator in domolibp2p

8bbfcd4

Make the pg test use the DOMO_DHT_TEST_DB env var if available

ce34b61

Split the behavior creation from the swarm setup

47d6527

Should make easier testing the network component using libp2p_swarm_test.

lu-zero and others added 11 commits August 21, 2023 18:51

Speedup the test execution

625a1bb

The resend task would not interfere with the rest this way.

fixup: Spurious comment

c56cf1b

fixup: is_syncronised is not async

4c9a15e

Do not keep the write lock longer than needed

691a18d

Explicitly yield in the swarm task

c9fc865

Compact the CI

7e3c3ef

Add a mean to access the list of peers

69056d7

Use more workers in the tests

5b4cadf

fixme: Check for a subset of peers

cc9ca28

Add Cache::get_hash

aca4ae7

Apply the impl Into<String>/owned Value pattern

6cf9414

Co-authored-by: Edoardo Morandi <morandidodo@gmail.com>

lu-zero force-pushed the dht-cache-reshape branch from 0bb9a1c to 6cf9414 Compare August 21, 2023 16:53

lu-zero added 2 commits August 21, 2023 19:32

Use futures-concurrency

5f79a70

Signal when the nodes are available, not when they are discovered

c4000eb

lu-zero force-pushed the dht-cache-reshape branch from 5c8cb37 to c4000eb Compare August 21, 2023 20:46

dodomorandi requested changes Aug 22, 2023

View reviewed changes

lu-zero added 8 commits August 22, 2023 10:09

Do not panic on non-utf8 data

b8a9410

use into_iter

d2e3540

Cleanup the peers check to be more readable

eef07a8

drop redundant drops

5f336c4

Avoid redundant path

33881b7

Use a worker task to persist the data

339272f

fixup: multiple_peer tests should wait for all of them

7020d08

Otherwise it would infiniloop it send the volatile message when only one of the two peers is ready.

Take ownership of the elem in the local cache API as well

c7f0568

dodomorandi approved these changes Aug 25, 2023

View reviewed changes

Refine the Ready peers event

37210e4

lu-zero force-pushed the dht-cache-reshape branch from 8849e61 to 37210e4 Compare September 11, 2023 10:46

dodomorandi and others added 3 commits September 28, 2023 09:11

fix: export structs from cache::local

4fd2dbe

feat!: overhaul dht_cache::cache::Query API

abff470

Mark Cache as Clone

ff3cc90

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reshape and document the dht-cache #36

reshape and document the dht-cache #36

lu-zero commented Jul 10, 2023

codecov bot commented Jul 20, 2023 •

edited

Loading

github-actions bot Aug 11, 2023

dodomorandi left a comment

dodomorandi Aug 21, 2023

lu-zero Aug 21, 2023

dodomorandi Aug 22, 2023

lu-zero Aug 22, 2023

dodomorandi Aug 21, 2023

dodomorandi Aug 21, 2023

dodomorandi Aug 21, 2023

dodomorandi left a comment

dodomorandi Aug 22, 2023

dodomorandi Aug 25, 2023

		pub fn with_uuid(mut self, uuid: &str) -> Self {
		self.uuid = Some(uuid.to_owned());

reshape and document the dht-cache #36

Are you sure you want to change the base?

reshape and document the dht-cache #36

Conversation

lu-zero commented Jul 10, 2023

codecov bot commented Jul 20, 2023 • edited Loading

Codecov Report

github-actions bot Aug 11, 2023

Choose a reason for hiding this comment

dodomorandi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dodomorandi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 20, 2023 •

edited

Loading